Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0] Fix: add missed open curly #61332

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 8, 2021

Backport of #61320 to release/6.0
Fixes #61282
/cc @buyaa-n @annchous

Customer Impact

A customer reported a regression in CustomAttributeTypedArgument.ToString() introduced in 6.0, the customer using it for a code generator where it was producing a valid C# code, which was broken by this bug, there is a workaround by implementing custom ToString() methods or updating the string produced, etc. but it is better to be fixed in the SDK. There could be more customers affected by this, we might get more bug reports after they port to the .Net 6.0 release. The fix is trivial, just adding the missing curly brace back to the string, nothing new added.

Testing

A new unit test was added that reproduces the bug and confirms the fix with the expected string produced by CustomAttributeTypedArgument.ToString()

Risk

Minimal, the only risk I could think of is if the customer did a workaround that updates the string result from CustomAttributeTypedArgument.ToString()​ that would be broken by this fix, but as this is fixed in 7.0 they will be broken again with 7.0

CC @jeffhandley @danmoseley

@ghost
Copy link

ghost commented Nov 8, 2021

Tagging subscribers to this area: @buyaa-n
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #61320 to release/6.0

/cc @buyaa-n @annchous

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@buyaa-n buyaa-n added the Servicing-consider Issue for next servicing release review label Nov 8, 2021
@buyaa-n buyaa-n added this to the 6.0.0 milestone Nov 8, 2021
@danmoseley
Copy link
Member

@buyaa-n what is the impact on the customer though? can you add to customer impact section: how commonly do folks do this, how many apps are broken, is there a workaround, etc. Or is this just a bug someone happened to spot but with low impact.

@buyaa-n
Copy link
Contributor

buyaa-n commented Nov 9, 2021

how commonly do folks do this, how many apps are broken, is there a workaround, etc. Or is this just a bug someone happened to spot but with low impact.

I would not expect it to be used much and would break many apps, correct me if I'm wrong @romkatv, how is this is impacting you? For me, this is low impact bug and I requested this for servicing only because it is a regression and the fix has no risk

what is the impact on the customer though? can you add to customer impact section:

Sure, i will update that section

@danmoseley
Copy link
Member

Thanks. Just being a regression isn't reason enough on its own to service shipped product. We look for evidence of sufficient impact, as well as risk.

@romkatv
Copy link

romkatv commented Nov 9, 2021

@romkatv, how is this is impacting you?

I maintain a code generator that, among other things, invokes CustomAttributeData.ToString(). This method produces valid C# code. Well, used to. Due to #61282 my code generator's output does not compile. I've worked around this bug by reimplementing CustomAttributeData.ToString(), CustomAttributeTypedArgument.ToString() and CustomAttributeNamedArgument.ToString().

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@buyaa-n
Copy link
Contributor

buyaa-n commented Nov 9, 2021

Thanks, @romkatv, I have updated the customer impact section accordingly

@buyaa-n buyaa-n modified the milestones: 6.0.0, 6.0.x Nov 12, 2021
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 12, 2021
@danmoseley danmoseley merged commit c074993 into release/6.0 Nov 12, 2021
@danmoseley danmoseley deleted the backport/pr-61320-to-release/6.0 branch November 12, 2021 22:25
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants